bug 1772163: Stop retieving instances by state tag filter on API call #269
Conversation
|
@enxebre: This pull request references Bugzilla bug 1772163, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@enxebre: This pull request references Bugzilla bug 1772163, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
size/L
|
@enxebre: This pull request references Bugzilla bug 1772163, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
enxebre
changed the title
This attemps to mitigate https://bugzilla.redhat.com/show_bug.cgi?id=1772163. Instead of telling the API request to filter by state, we fetch all and discriminate only terminated instances
enxebre
force-pushed
the
stop-fetching-by-state
branch
from
f8c0630 to
c8e341f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bison The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@enxebre: All pull requests linked via external trackers have merged. Bugzilla bug 1772163 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Oh I missed those PRs @wking! sorry and thanks! I just had a look and I think both should be covered here but please rebase if you think otherwise. Thanks a bunch! |
…ancesOutput
So we can create pending, terminated, etc. instances for testing.
This should scale better than
stub{State}InstanceDescribeInstancesOutput from
openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving
instances by state on API call, 2019-11-14,
openshift#269).
…ceByID openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving instances by state on API call, 2019-11-14, openshift#269) did part of this. This commit: * Restores the instanceStateFilter argument to getInstanceByID, to avoid having two definitions (existingInstanceStates and a local != Terminated in getExistingInstanceByID) that define what it means to exist. This will also give us logged error messages like: instance i-123 state terminated is not in running, pending, stopped, stopping, shutting-down which will make it easier to find and fix omissions in existingInstanceStates. * Adds a -running suffix to has-status-search-by-id, so it's easier to distinguish from the terminated case. * Converts to the hyphenated has-status-search-by-id-terminated, instead of mixing hyphens and spaces in a case name. * Uses inline argument for the DescribeInstances mocks, so we don't have to use request and request2. These aren't so big that inlining them makes the mock setup unreadable. * Returns an empty set of instances in the instance-listing fallback call in has-status-search-by-id-terminated (which we now require to happen after the by-ID call). The list call is filtered by state, so it wouldn't return a terminated instance.
Rebased each of them :). |
…ancesOutput
So we can create pending, terminated, etc. instances for testing.
This should scale better than
stub{State}InstanceDescribeInstancesOutput from
openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving
instances by state on API call, 2019-11-14,
openshift#269).
…ceByID openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving instances by state on API call, 2019-11-14, openshift#269) did part of this. This commit: * Restores the instanceStateFilter argument to getInstanceByID, to avoid having two definitions (existingInstanceStates and a local != Terminated in getExistingInstanceByID) that define what it means to exist. This will also give us logged error messages like: instance i-123 state terminated is not in running, pending, stopped, stopping, shutting-down which will make it easier to find and fix omissions in existingInstanceStates. * Adds a -running suffix to has-status-search-by-id, so it's easier to distinguish from the terminated case. * Converts to the hyphenated has-status-search-by-id-terminated, instead of mixing hyphens and spaces in a case name. * Uses inline argument for the DescribeInstances mocks, so we don't have to use request and request2. These aren't so big that inlining them makes the mock setup unreadable. * Returns an empty set of instances in the instance-listing fallback call in has-status-search-by-id-terminated (which we now require to happen after the by-ID call). The list call is filtered by state, so it wouldn't return a terminated instance.
This attempts to mitigate https://bugzilla.redhat.com/show_bug.cgi?id=1772163.
Instead of telling the API request to filter by state tag, we fetch all and discriminate only terminated instances client side.